Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add blacklist support #269

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

Formaily
Copy link

@Formaily Formaily commented Sep 8, 2014

No description provided.

@Formaily
Copy link
Author

Formaily commented Sep 8, 2014

Thanks @dryabov's review and advice, but I think unfollow action need confirmed by user. That means it should be done on client side.
Let me know if I missed something.

@dryabov
Copy link
Contributor

dryabov commented Nov 14, 2014

@miguelfreitas Do you have objections against merging this patch?

@miguelfreitas
Copy link
Owner

looks like i've completely missed this.
what's the point exactly? i mean, how is it different to add a given user name to both "following" and "ignore" list compared to not adding it at all?

@dryabov
Copy link
Contributor

dryabov commented Nov 14, 2014

Currently it just stops user's torrent, but we can remove user from m_following automatically. At least let's start to discuss what to improve there.

@Formaily I saw you sent corresponding patch to twister-html, but to XXCOM/twister-html repository XXCOM/twister-html@09b2a3a and not to https://github.com/miguelfreitas/twister-html. Are you going to make pull request to https://github.com/miguelfreitas/twister-html as well?

@miguelfreitas
Copy link
Owner

no, i'm asking why add the username to both lists instead of just removing from the one of them.
what is the advantage of this increased complexity?

@dryabov
Copy link
Contributor

dryabov commented Nov 14, 2014

The idea of blacklist is to filter hashtags and mentions from spam messages (by other users).

@miguelfreitas
Copy link
Owner

how? you are just not subscribing to his torrent. filtering hashtags and mentions is basically dht.

(sidenote: i'd like to improve mentions though by creating a new api which collect mentions from users you follow. this way, mentions from your friends can't get lost/overridden by an anonymous attacker which sends tons of mentions in your name)

@miguelfreitas
Copy link
Owner

besides, if i want to block spam messages from an specific user the last thing i'd do would be to follow him :-)

@dryabov
Copy link
Contributor

dryabov commented Nov 14, 2014

It is unrelated to follow/unfollow mechanism. But there is a case if you follow a user, and at some point you choose to block it, and seems in this case it is reasonable to unfollow that user automatically.

As to implementation, I'd add check of strUsername against blacklist in verifySignature function, and seems it gives us desired behavior (node_impl::incoming_request calls it to validate message).

@miguelfreitas
Copy link
Owner

Exactly. If we choose to just unfollow then we don't need to patch twister-core.

Btw, if you are trying to block a given hashtag why not block the tag instead of the user(s) sending it?

@dryabov
Copy link
Contributor

dryabov commented Nov 14, 2014

We can have it both ways, blacklist for usernames and other one for hashtags. But IMHO blacklist of usernames is more important feature.

@miguelfreitas
Copy link
Owner

I'm not against blacklisting usernames (so we don't hear from them again). But i fail to understand why you need to patch twister-core instead of just removing the follow with the already existing api.

blacklisting the propagation of dht content for a specific user at your node doesn't sound much effective imho. with a big twister network, the probability that this specific user's posts are hosted on your node will be pretty low. even if your node happen to be neighbor of that particular dht key, there are still 7 more random nodes which would happily host the content.

@dryabov
Copy link
Contributor

dryabov commented Nov 14, 2014

Maybe I'm mistaken, but seems that both putData (incoming) and getData (response) DHT messages should be blocked (in node_impl::incoming_request and dht_get_observer::reply, correspondingly).

Related previous discussions:
#215
#264

@miguelfreitas
Copy link
Owner

but node_impl::incoming_request and dht_get_observer::reply don't depend on the torrents you join.

if you are referring to possibly hacking verifySignature, yes, that would probably block them (this is not in the current patch we are discussing though).

care must be taken with validation of pieces. i don't remember if we validate the original signature for a RT post, for example. if we do, a RT of an originally blocked user might trigger a validation failure in another user's torrent. one peer which insists in sending pieces that are not considered valid by the other peer will get banned in libtorrent. that is:

  • userB follows userA
  • postBySpammer is retransmitted by userA.
  • userB blocks Spammer
  • userA will try to send RT(postBySpammer) to userB using his own torrent
  • userB would refuse to validate this post
  • userA would keep trying (he believes it is a valid RT)
  • userB bans userA's node in libtorrent

i'm not saying this will happen, it all depends on validation checks we do. i'm just saying we must be careful with side effects like that.

@dryabov
Copy link
Contributor

dryabov commented Nov 14, 2014

OK, I see. Then let's add isBlacklisted(string username) method to twister.h/cpp and call it from dht_get_observer::reply. putData message will work as expected (that is if a user blacklist bbc_world account for personal choices, we [imho] shouldn't prevent storing bbc_world's messages on user's node).

@miguelfreitas
Copy link
Owner

ok, we may even do that. but you are aware this is mostly useless, right?

  1. preventing putData on your node won't prevent the posts from propagating (by other nodes).
  2. and filtering at dht_get_observer::reply is no different than twister-html doing it entirely in frontend.

@dryabov
Copy link
Contributor

dryabov commented Nov 14, 2014

Right, filtering can be done client-side (server-side it would be faster and client-independent, but I would prefer light-weight server in flavor of multi-featured one). So, we can add check to _dhtgetProcessPending, but there is a question: Where to store blacklist?

  1. Store in localStorage: no synchronization (even between clients connected to the same twisterd server), the list will be lost after switch to another browser.
  2. Store on the server: no synchronization.
  3. Store in DHT, plain text (like followingN): can be synchronized, but is it ok to make list readable? If yes, we get a nice feature to read blacklists of other users (as suggested by @iShift).
  4. Store in DHT, encrypted: a server-side code for encryption is necessary.

@miguelfreitas
Copy link
Owner

Agreed! Another reason I like for client side things (where reasonable) is that it is easier and faster to experiment different strategies for things that don't have a single or closed solution.

I'd suggest going with (3) first and then moving to (4) as soon as we add necessary server-side support. Clients can easily migrate and even make it optional user decide whether to encrypt or not.

Plain text option (3) may help building automatic spam protection: if X% of your friends have blocked the same given username you may trust to simply ignore this username as well.

@dryabov
Copy link
Contributor

dryabov commented Nov 15, 2014

I'd suggest to use both DHT and server's storage, just because of DHT doesn't provide consistency. So, anyway commands like ignore/unignore/getignoring should be implemented server-side.

@miguelfreitas
Copy link
Owner

What if instead we provided a command to store arbitrary per-user settings, in the form of a json structure?
Then we won't need to add new commands for every new bit of information we want to keep.

@dryabov
Copy link
Contributor

dryabov commented Nov 15, 2014

Yes, sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants